-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added a JewishDate
constructor that takes sunset into account
#193
Added a JewishDate
constructor that takes sunset into account
#193
Conversation
Seemed to be a big pain point for many users, including myself. Super easy to implement ourselves, but might as well have a method that does it for us.
JewishDate
constructor that takes sunset into account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@compugenius ,
This seems a valuable change and I appreciate your effort. I have a number of comments.
- This change locks in sunset as the date change point, while some would want various tzais times for this. Why not pass a Date object as opposed to the ComplexZmanimCalendar (in theory some would be using the ZmanimCalender or even the AstronomicalCalendar for this, and you now force them to use this specific calendar), as well as force sunset (some may want to use sea level sunset and other elevation based). In short, changing to a Date would seem to make this simpler and much more flexible.
- The change forces you to reinstantiate /construct the JewishDate for every new day. What happens in scenarios like a user changing the date by the various setJewishDate() where an overridden version for these would allow passing a date as well to forward the date if it is after sunset/tzais to allow the HebrewDate to be one day forward.
- It would forcibly roll the Gregorian date ~6 hours early. This would potentially lead to issues.
- Your implementation of resetDateWithSunset() only works on the current date. What If I instantiated a JewishDate today for 1 Tishrei, calling this public method would be confusing to say the least. Yes, I know the JavaDocs mention this, but I think that many will find it confusing. You may want to think of a way to allow support of this valuable addition to work on other dates as well.
- I would also suggest that the resetDateWithSunset be renamed to something like forwardDateAtJewishStartTime() or something similar.
@KosherJava
I will say that I tried creating a copy of the file and editing it in Android Studio to test my changes, and I got a ton of errors, even after importing your package and changing the references. |
My idea would be to incorporate Gregorian hour/minute/second into the jewish date calculations throughout the JewishDate. The constructor could take a sunset time instead of a ComplexZmanimCalendar, which would allow for more freedom in choosing how the sunset is calculated (ex: it can default to midnight for when the user's location isn't available). I don't think it would be difficult to incorporate into the core JewishDate functionality, and it would allow for potentially removing workarounds throughout the rest of the codebase (like having different tefila rules for during the day vs maariv). I'm working on refactoring the Dart port of this library and I'm planning on making these changes to JewishDate there. |
@DanielSmith1239, |
Seemed to be a big pain point for many users, including myself. Super easy to implement ourselves, but might as well have a method that does it for us.